Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support concurrency limit #541

Closed
wants to merge 3 commits into from
Closed

feat: support concurrency limit #541

wants to merge 3 commits into from

Conversation

thep0y
Copy link

@thep0y thep0y commented Jan 12, 2024

Copy link

vercel bot commented Jan 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
upload ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 13, 2024 3:58am

@afc163
Copy link
Member

afc163 commented Jan 12, 2024

@MadCcc 看看

Copy link

codecov bot commented Jan 12, 2024

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (6027d4c) 87.21% compared to head (050b5fb) 82.91%.

❗ Current head 050b5fb differs from pull request most recent head bae65e1. Consider uploading reports for the commit bae65e1 to get more accurate results

Files Patch % Lines
src/asyncPool.ts 0.00% 13 Missing ⚠️
src/AjaxUploader.tsx 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #541      +/-   ##
==========================================
- Coverage   87.21%   82.91%   -4.30%     
==========================================
  Files           6        7       +1     
  Lines         266      281      +15     
  Branches       72       76       +4     
==========================================
+ Hits          232      233       +1     
- Misses         34       48      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@afc163
Copy link
Member

afc163 commented Jan 12, 2024

看上去得补一些用例,测试覆盖率下降了。

@@ -69,6 +69,7 @@ React.render(<Upload />, container);
|accept | string | | input accept attribute |
|capture | string | | input capture attribute |
|multiple | boolean | false | only support ie10+|
|concurrencyLimit | number | | asynchronously posts files with the concurrency limit |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concurrencyLimit 的默认值是多少?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

默认值是 undefined,即不使用 concurrencyLimit

const tasks: Promise<U>[] = [];
const pendings: Promise<U>[] = []


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

多了一个空行。

src/asyncPool.ts Outdated

if (concurrencyLimit <= items.length) {
task.then(() => {
pendings.splice(pendings.indexOf(task), 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

分号都补一下。

@thep0y
Copy link
Author

thep0y commented Jan 13, 2024

早上试了一下,xhr 请求虽然是异步的不受异步控制,asyncPool没有发挥作用,需要使用别的方式实现,是否考虑将请求方法改用fetch

Although `XMLHttpRequest` can asynchronously send requests, this process is implicit and cannot be awaited through `await`. To control the concurrency of `XMLHttpRequest`, it is necessary to convert `XMLHttpRequest` into an explicit asynchronous task. Modify the task completion status in both `xhr.onload` and `xhr.onerror` to enable `asyncPool` to remove completed `pending` requests through `Promise.race` and initiate the next request.

To minimize code changes, add a `requestTasks` queue in the component to store `XMLHttpRequest` request tasks. Additionally, include an `appendRequestTask` parameter in the `request` function to add tasks to the queue.

When the `concurrencyLimit` property is provided to the component, use `asyncPool` to control the concurrency and initiate requests. Otherwise, sequentially initiate all requests in the `requestTasks` queue.
@afc163
Copy link
Member

afc163 commented Jan 13, 2024

  1. xhr 改 fetch 变动会比较大,我不建议为了加新特性动底层。
  2. 上传逻辑的自定义最灵活的方式是 customRequest,所以是不是做个内置的 concurrencyRequest 方便开发者直接使用就好?

@thep0y
Copy link
Author

thep0y commented Jan 13, 2024

  1. xhr 改 fetch 变动会比较大,我不建议为了加新特性动底层。
  2. 上传逻辑的自定义最灵活的方式是 customRequest,所以是不是做个内置的 concurrencyRequest 方便开发者直接使用就好?

方式 2 确实可能更好。

@thep0y
Copy link
Author

thep0y commented Jan 13, 2024

换种方式实现

@thep0y thep0y closed this Jan 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants